Clear idp cookie after succesful SSO#47569
Conversation
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
This PR updates the end-user OTA enrollment flow to prevent reusing a stale IdP identity across enrollments by clearing the BYOD IdP cookie after successful SSO, and by plumbing an IdP UUID into the Android enrollment token request when needed.
Changes:
- Clears the BYOD IdP cookie after creating an Android enrollment token (via
SetCookieson the response). - Passes an
IdpUUIDvalue through the/enrollHTML template and into the Android enrollment token fetch asidp_uuid. - Updates the Android enrollment token request decoder to accept
idp_uuidfrom the query string before falling back to the cookie.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| server/service/frontend.go | Passes IdpUUID into the enroll template and clears the BYOD IdP cookie before rendering the enroll page (needs adjustment to avoid breaking iOS/iPadOS flows). |
| server/mdm/android/service/service.go | Accepts idp_uuid query param for enrollment token requests before checking the cookie. |
| server/mdm/android/service.go | Adds SetCookies to clear the BYOD IdP cookie after successfully creating an enrollment token; introduces a cookie name constant. |
| frontend/templates/enroll-ota.html | Appends idp_uuid to the Android enrollment token fetch URL when present. |
| changes/47343-idp-cookie | User-visible change entry (content excluded from review). |
Files excluded by content exclusion policy (1)
- changes/47343-idp-cookie
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #47569 +/- ##
==========================================
- Coverage 67.19% 67.18% -0.01%
==========================================
Files 3616 3616
Lines 229016 229051 +35
Branches 11785 11931 +146
==========================================
+ Hits 153887 153890 +3
- Misses 61302 61326 +24
- Partials 13827 13835 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR fixes fully-managed Android enrollment by threading an IdP UUID through the enrollment flow. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/mdm/android/service.go (1)
10-10: ⚡ Quick winUse the shared BYOD IdP cookie constant instead of duplicating the literal.
byodIdpCookieNameduplicates a cross-layer contract value. If the canonical cookie name changes, this cookie-clearing path can silently break.Proposed change
import ( "context" "net/http" + shared_mdm "github.com/fleetdm/fleet/v4/pkg/mdm" "google.golang.org/api/androidmanagement/v1" ) - -const byodIdpCookieName = "__Host-FLEETBYODIDP" @@ http.SetCookie(w, &http.Cookie{ - Name: byodIdpCookieName, + Name: shared_mdm.BYODIdpCookieName, Value: "", Path: "/",🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/mdm/android/service.go` at line 10, The constant byodIdpCookieName is currently defined as a local literal value "__Host-FLEETBYODIDP" in this file, which duplicates the canonical definition elsewhere in the codebase. Remove the local constant definition and instead import and use the shared byodIdpCookieName constant from its canonical location (likely in a shared utilities or constants package) to ensure consistency across layers and prevent the cookie-clearing logic from breaking if the canonical cookie name is updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/mdm/android/service/service.go`:
- Around line 500-506: The idp_uuid query parameter is being accepted directly
without server-side validation, allowing any caller with an enroll_secret to
bind an enrollment to any IdP UUID without fresh authentication. Remove or
significantly restrict the code block that extracts idp_uuid from the query
parameter and directly populates it in the enrollmentTokenRequest struct.
Instead, only accept idp_uuid when it is cryptographically bound to
server-trusted state, such as validation against an active session cookie or a
signed one-time token generated by the server during a prior authenticated
operation. Ensure that the enrollmentTokenRequest construction validates the
identity proof before incorporating any IdP identifier.
---
Nitpick comments:
In `@server/mdm/android/service.go`:
- Line 10: The constant byodIdpCookieName is currently defined as a local
literal value "__Host-FLEETBYODIDP" in this file, which duplicates the canonical
definition elsewhere in the codebase. Remove the local constant definition and
instead import and use the shared byodIdpCookieName constant from its canonical
location (likely in a shared utilities or constants package) to ensure
consistency across layers and prevent the cookie-clearing logic from breaking if
the canonical cookie name is updated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e13046bb-cb8e-4cf9-85df-43df31ff1b85
📒 Files selected for processing (5)
changes/47343-idp-cookiefrontend/templates/enroll-ota.htmlserver/mdm/android/service.goserver/mdm/android/service/service.goserver/service/frontend.go
There was a problem hiding this comment.
Warning
- Copilot's review of this pull request may be incomplete because some of the changed files are excluded by your Copilot content exclusion settings. See Excluding content from Copilot for details.
Pull request overview
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
Files excluded by content exclusion policy (1)
- changes/47343-idp-cookie
| // Clear the BYOD IdP cookie now that we are about to render the enrollment page. | ||
| var idpUUID string | ||
| fullyManaged := r.URL.Query().Get("fully_managed") | ||
| if authRequired && fullyManaged == "true" { |
| // Clear the BYOD IdP cookie now that we are about to render the enrollment page. | ||
| var idpUUID string | ||
| fullyManaged := r.URL.Query().Get("fully_managed") | ||
| if authRequired && fullyManaged == "true" { | ||
| idpUUID = r.URL.Query().Get("enrollment_reference") | ||
| http.SetCookie(w, &http.Cookie{ | ||
| Name: shared_mdm.BYODIdpCookieName, | ||
| Value: "", | ||
| Path: "/", | ||
| MaxAge: -1, | ||
| Secure: cookieSecure, | ||
| HttpOnly: true, | ||
| SameSite: http.SameSiteLaxMode, | ||
| }) | ||
| } |
| if idpUUID := r.URL.Query().Get("idp_uuid"); idpUUID != "" { | ||
| return &enrollmentTokenRequest{ | ||
| EnrollSecret: enrollSecret, | ||
| IdpUUID: idpUUID, | ||
| FullyManaged: fullyManaged, | ||
| }, nil | ||
| } |
Related issue: Resolves #47343
Checklist for submitter
If some of the following don't apply, delete the relevant line.
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Testing
Summary by CodeRabbit